[FIX] The first TODO in avc_functions.c and FIXME in avc_functions.c #1695
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR addresses two specific items in the AVC functions library: a TODO comment about handling broken NAL units and a FIXME comment about calling set_fts for SEI NAL units. The changes improve error handling for corrupted AVC/H.264 streams and ensure proper timestamp handling for SEI units.
- Enhanced error messaging for corrupted NAL units with detailed explanations
- Enabled set_fts function call for SEI NAL units to fix timestamp handling
- Updated changelog to document the fixes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/lib_ccx/avc_functions.c | Improved error message for corrupted NAL units and enabled set_fts call for SEI units |
| docs/CHANGES.TXT | Added changelog entry documenting the TODO and FIXME fixes |
Comment on lines
+100
to
+103
| mprint("\rWarning: Invalid prevention bytes detected in NAL unit type %u (0x%02X). " | ||
| "This NAL unit contains an illegal byte sequence (0x000000, 0x000001, or 0x000002) or " | ||
| "improper prevention byte (0x03). " | ||
| "This may indicate a corrupted AVC/H.264 stream. NAL unit skipped.\n", |
There was a problem hiding this comment.
The error message incorrectly describes the failure condition. The remove_03emu function can fail for reasons other than invalid prevention bytes, such as general parsing errors or memory issues. Consider using a more generic message like 'Failed to process NAL unit type %u (0x%02X) due to emulation prevention byte removal failure.'
Suggested change
| mprint("\rWarning: Invalid prevention bytes detected in NAL unit type %u (0x%02X). " | |
| "This NAL unit contains an illegal byte sequence (0x000000, 0x000001, or 0x000002) or " | |
| "improper prevention byte (0x03). " | |
| "This may indicate a corrupted AVC/H.264 stream. NAL unit skipped.\n", | |
| mprint("\rWarning: Failed to process NAL unit type %u (0x%02X) due to emulation prevention byte removal failure. " | |
| "This may indicate a corrupted AVC/H.264 stream or an internal processing error. NAL unit skipped.\n", |
Copilot uses AI. Check for mistakes.
Comment on lines
+101
to
+102
| "This NAL unit contains an illegal byte sequence (0x000000, 0x000001, or 0x000002) or " | ||
| "improper prevention byte (0x03). " |
There was a problem hiding this comment.
The error message contains technical inaccuracies. The sequences 0x000000, 0x000001, and 0x000002 are not inherently illegal in H.264 streams - 0x000001 is actually the standard start code prefix. The message should focus on the actual failure rather than speculating about specific byte patterns.
Suggested change
| "This NAL unit contains an illegal byte sequence (0x000000, 0x000001, or 0x000002) or " | |
| "improper prevention byte (0x03). " | |
| "This NAL unit contains improper prevention bytes (0x03) or other anomalies. " |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I think this suggestion is incorrect, The comment at Line 274 states that 0x000000, 0x000001 or 0x000002 shall not occur at any byte-aligned position
Comment on lines
105
to
+110
| return; | ||
| } | ||
|
|
||
| dvprint("BEGIN NAL unit type: %d length %d ref_idc: %d - Buffered captions before: %d\n", | ||
| nal_unit_type, NAL_stop - NAL_start - 1, dec_ctx->avc_ctx->nal_ref_idc, !dec_ctx->avc_ctx->cc_buffer_saved); | ||
|
|
There was a problem hiding this comment.
Moving the dvprint statement after the error check means debug information won't be printed for NAL units that fail emulation prevention byte removal. This could make debugging more difficult. Consider keeping the debug print before the error check or adding a separate debug message for failed NAL units.
Suggested change
| return; | |
| } | |
| dvprint("BEGIN NAL unit type: %d length %d ref_idc: %d - Buffered captions before: %d\n", | |
| nal_unit_type, NAL_stop - NAL_start - 1, dec_ctx->avc_ctx->nal_ref_idc, !dec_ctx->avc_ctx->cc_buffer_saved); | |
| dvprint("Failed to process NAL unit type: %d - Emulation prevention byte removal failed.\n", | |
| nal_unit_type); | |
| return; | |
| } |
Copilot uses AI. Check for mistakes.
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 099fa05...:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
All tests passing on the master branch were passed completely. Check the result page for more info. |
|
@prateekmedia I have AVC Functions in 11th week of my Plan, should we scrap this PR as the code will be redundant anyway? |
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit 81fdecd...:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
All tests passing on the master branch were passed completely. Check the result page for more info. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
This PR resolves one TODO and one FIXME in the avc_functions.c.
The first TODO was -
TODO: Do something if newsize == -1 (broken NAL)So, what my changes do is, print out a more verbose error message.
The first FIXME was to check the set_fts function for
CCX_NAL_TYPE_SEIwhich was only being checked in slice_header for NAL Type asCCX_NAL_TYPE_CODED_SLICE_IDR_PICTUREorCCX_NAL_TYPE_CODED_SLICE_NON_IDR_PICTURE_1.There are more TODOs in the AVC Functions library, which need to be resolved before or during the porting to Rust.
This PR will help in debugging and resolving the AVC Issues currently prevalent in CCExtractor.( #1626 , #1597 , #1592 )